Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: nicer tracebacks in run mode on scripts #1191

Merged
merged 5 commits into from
Jan 16, 2024
Merged

fix: nicer tracebacks in run mode on scripts #1191

merged 5 commits into from
Jan 16, 2024

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Jan 5, 2024 •
edited
Loading

This is one way to improve the tracebacks as reported in #1187 and mentioned in #1180.

  • I have added an entry to docs/changelog.md

Summary of changes

This is the simplest way I initially see to get filenames. Instead of passing around the contents of the script as a string, scripts are passed around as Paths, which allows the run command to run the path rather than passing the string on the command line.

Test plan

If you start with this script:

import httpx

You get this:

$ pipx run ./script.py
Traceback (most recent call last):
  File "<string>", line 1, in <module>
    import httpx
ModuleNotFoundError: No module named 'httpx'

After this PR, you get this:

$ pipx run ./script.py
Traceback (most recent call last):
  File "/Users/henryschreiner/git/software/pipx/script.py", line 1, in <module>
    import httpx
ModuleNotFoundError: No module named 'httpx'

@henryiii henryiii force-pushed the henryiii/fix/runtb branch 2 times, most recently from c8bb144 to 6aec4e9 Compare January 5, 2024 16:46
@henryiii henryiii marked this pull request as ready for review January 5, 2024 16:46
@henryiii henryiii mentioned this pull request Jan 5, 2024
Merged
flying-sheep
flying-sheep suggested changes Jan 5, 2024
View reviewed changes
Copy link
Contributor

@flying-sheep flying-sheep left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks lovely! I think there could be some slight deduplication, otherwise wonderful!

My motivation for this is that people can now use this pipx functionality in scripts instead of directly, in which case this PR’s changes will come in handy for debugging.

henryiii
henryiii commented Jan 5, 2024
View reviewed changes
chrysle
chrysle previously approved these changes Jan 5, 2024
View reviewed changes
Copy link
Contributor

@chrysle chrysle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@chrysle
Copy link
Contributor

chrysle commented Jan 5, 2024

I think we should really enable merge queues, this constant branch updating is annoying.

@henryiii henryiii force-pushed the henryiii/fix/runtb branch from a1fd530 to 19d9e62 Compare January 5, 2024 21:07
@rsalmaso
Copy link

rsalmaso commented Jan 7, 2024

Happy for this patch!

Running script instead of string solve another problem: run the script via shebang #!/usr/bin/env -S pipx run and get the correct filename sys.argv[0].

dukecat0
dukecat0 requested changes Jan 7, 2024
View reviewed changes
Copy link
Member

@dukecat0 dukecat0 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test please

@gaborbernat gaborbernat marked this pull request as draft January 10, 2024 20:53
@henryiii
Copy link
Contributor Author

henryiii commented Jan 10, 2024 •
edited
Loading

What should this do if a URI is passed, by the way? Currently that still says "<string>". That could be a future improvement, but something to think about.

@dukecat0
Copy link
Member

What should this do if a URI is passed, by the way? Currently that still says "". That could be a future improvement, > but something to think about.

Perhaps we can have an additional check here?

pipx/src/pipx/commands/run.py

Line 57 in 5563ae6

if urllib.parse.urlparse(app).scheme:

@henryiii henryiii marked this pull request as ready for review January 12, 2024 16:22
@henryiii
Copy link
Contributor Author

henryiii commented Jan 12, 2024 •
edited
Loading

A check wouldn't help, we are already doing the "right" think (since when getting a URI it needs to be downloaded, possibly from a remote source), it's just then a question of if there's a way to get it to show something nicer than "<string>". This was fixable since loading a local file can just be directly run by the python command. I would assume there's a way to customize running on arbitrary input, since things like IPython do it (though at in least some cases, they write out temporary files).

IMO the big one to fix though is the one in this PR, with URI's being a second, smaller improvement later. I'd expect running on a local file to give the right traceback, not sure I'd expect it from an arbitrary URI.

gaborbernat
gaborbernat requested changes Jan 14, 2024
View reviewed changes
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rebase and add changelog.

@gaborbernat gaborbernat merged commit 18a0784 into pypa:main Jan 16, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flying-sheep flying-sheep flying-sheep requested changes

@gaborbernat gaborbernat gaborbernat requested changes

@dukecat0 dukecat0 dukecat0 approved these changes

@chrysle chrysle chrysle left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@henryiii @chrysle @rsalmaso @dukecat0 @flying-sheep @gaborbernat